notification-service: accept epoch timestamps and drop poison SQS messages - #1544
devin-ai-integration[bot] wants to merge 1 commit into
Conversation
…sages The consumer could switch to a strict JSON parser (Redis chaos flag) that rejected legacy events carrying epoch-int timestamps; unparseable messages were never deleted, so they cycled through the visibility timeout and the queue grew without bound. - Decode SqsNotificationMessage.timestamp via EventTimestampSerializer, accepting RFC 3339 strings or epoch seconds/millis regardless of parser strictness. - Remove the strict-parser toggle and the jedis dependency it needed. - Delete messages that fail deserialization (redelivery can't fix them) and count processEvent exceptions in notifications.processing.errors. - Fill in the runbook investigation/resolution steps.
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| return try { | ||
| // Try parsing as direct message first | ||
| parser.decodeFromString<SqsNotificationMessage>(body) | ||
| json.decodeFromString<SqsNotificationMessage>(body) |
There was a problem hiding this comment.
🟡 Notification failure demo becomes inert
Every notification-schema injection now leaves parseMessage lenient because it never reads the Redis flag. The scenario catalog still offers this planted failure, so demos report success without breaking notification processing.
Prompt for agents
Restore the notification-schema planted failure while retaining normal support for epoch timestamps. SqsConsumer.parseMessage must consult chaos:notification-service:consumer_strict_schema and reject legacy numeric timestamps only when that scenario is active. Keep normal production parsing compatible with epoch seconds and milliseconds. Preserve scripts/bug-catalog.yaml, inject-bug.sh, the admin dashboard button, and the alert-driven demo contract. If this remediation belongs only on a workshop variant rather than main, move the change to that branch instead of changing the golden app.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentional and called out in the PR description: the strict-parser toggle is the root cause of the incident this PR remediates, so the remediation necessarily makes the notification-schema scenario inert. Whether that belongs on main (golden-app policy) or only on a workshop-* variant branch is a call for the maintainers — I've asked in the session. If the answer is "keep the lab", I'll retarget this PR at a workshop branch rather than re-adding a flag-gated failure path to production code.
Summary
Fix for incident Notification SQS consumer failing to process messages (
NotificationConsumerProcessingErrors).Root cause (
services/notification-service/.../consumer/SqsConsumer.kt):parseMessageswapped in a strictJson { isLenient = false }parser whenever the Redis flagchaos:notification-service:consumer_strict_schemaexisted.SqsNotificationMessage.timestampis typedString, so legacy events carrying an epoch number ("timestamp": 1704067200) only decoded because the lenient parser coerces unquoted literals to strings; the strict parser threwSerializationException. Failed messages were logged and counted but never deleted, so they re-entered the queue after the 60s visibility timeout and depth grew without bound.Changes
SqsNotificationMessage.timestampis now decoded viaEventTimestampSerializer, which accepts an RFC 3339 string or an epoch number (seconds or millis, split at1e11) and normalises to an ISO-8601 UTC string. Works with any parser configuration.chaos:notification-service:consumer_strict_schemalookup from the consumer, along with the now-unusedjedisdependency.startPolling:processEventexceptions were not counted innotifications.processing.errors.runTestpolling test asserting the unparseable message is deleted while the valid one is processed.docs/runbooks/notification-processing-failure.md.Note for reviewers (golden-app policy): this removes the code path behind the
notification-schemascenario inscripts/bug-catalog.yaml/ the admin-dashboard chaos button. Setting the flag becomes a no-op. If the lab must be preserved onmain, keep this PR as the reference remediation on aworkshop-*branch instead of merging.Verified:
./gradlew buildinservices/notification-service(38 tests, 0 failures). No live environment was available locally, so the diagnosis is from code/runbook rather than pod logs.Link to Devin session: https://partner-workshops.devinenterprise.com/sessions/834333f5757741e58a460024e6844856
Open in Devin Desktop: https://partner-workshops.devinenterprise.com/desktop/session/834333f5757741e58a460024e6844856?variant=devin